-
Notifications
You must be signed in to change notification settings - Fork 98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
stroke style directive #17
stroke style directive #17
Conversation
04f9b72
to
cc9ff46
Compare
Rebased and ready for review. |
cc9ff46
to
ef39a94
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, @FallenRiteMonk ?
@@ -1,5 +1,7 @@ | |||
import { Component, Directive, EventEmitter, Host, OnDestroy, OnChanges, AfterContentInit, Input, Output, ContentChild } from '@angular/core'; | |||
import { style } from 'openlayers'; | |||
import { Subscription } from 'rxjs/Subscription'; | |||
import { Observable } from 'rxjs/Observable'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import Observable
if (this.iconStyleDirective) { | ||
this.childSubscription$ = this.iconStyleDirective.onChanged.subscribe((): void => this.update()); | ||
} | ||
else if (this.strokeStyleDirective) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can stroke and icon not be used on the same style element?
If they can, this shouldn't be else if
but only if
same for the update function
The code looks good to me, but wasn't able to test it due to the lack of an example. |
Addresses comments above. |
LGTM, I would also say that you can keep the relative path as a temporary solution since its more important that everybody can test the example than it being aot compatible, especially since we know that library is aot compatible at the moment. Then when we finished discussing the real solution in #30 we can change it again. |
39236ed
to
6cc7c62
Compare
Cool. I will leave the rel path then. |
should be addressed in 0.5.0, though in exactly the same fashion. Feel free to reopen if need be. |
No description provided.